feat(api): add scope-guarded profile write endpoints - #585

@haofeif @fanhongy please re-review this PR.

Thanks, both confirmed and fixed. I reproduced each before changing anything.

P2, write validation can persist profiles the runtime cannot load

Confirmed exactly as described. Both shapes produce zero validator findings,
persist, then fail to load:

mcpServers non-string key    validator errors: 0   would persist: True
                             later load: ValidationError  mcpServers.1.[key]  Input should be a valid string
toolAliases non-string key   validator errors: 0   would persist: True
                             later load: ValidationError  toolAliases.1.[key]
control: valid profile       validator errors: 0   would persist: True   later load: OK

The mismatch is format-level: the body is YAML, which allows any scalar as a
mapping key, and the gate is JSON Schema, where object keys are strings by
definition. Your framing of ownership is right too. The schema limitation
predates this PR; what this PR did was make that schema the sole gate in front of
a new persistence operation, which turned a latent gap into a broken save path.

validate_frontmatter now walks the parsed document and reports any non-string
mapping key as an error. Two choices worth surfacing:

It lives in the validator, not only on the write path. Otherwise
cao profile validate and POST /agents/profiles/validate would call such a
document valid while the write routes reject it, and the UI in PR C validates
before saving, so it would show a contradiction. That does mean this PR now
touches the #575 service, which I'd rather state than have you find.

It checks the key type generally rather than enumerating the object-valued
fields. YAML auto-types more than integers: an unquoted 2026-01-01: key becomes
a datetime.date and fails identically. That case is in the tests.

On your first suggestion, validating the parsed metadata through AgentProfile:
I looked at it and did not take it, because it catches strictly more but collides
with the unresolved-text decision from point 1. The write path persists
unresolved content, so model-validating unresolved would reject
provider_init_timeout: ${TIMEOUT} that the runtime accepts after
resolve_env_vars. Model-validating resolved content instead makes acceptance
depend on the server's environment, so the same document validates on one machine
and not another. Neither is obviously right, so it is a disclosed gap in the
description rather than a decision made here. Say the word if you'd rather it
land in this PR.

Endpoint regressions added as you asked: 400 with nothing on disk for POST, and
400 with the original surviving byte-for-byte for PUT, across all three key
shapes. Plus one asserting the rejected document genuinely fails
parse_agent_profile_text, so the rule reads as a reason rather than an
arbitrary restriction.

P3, rejection-shape test

Correct, and worse than one route. Enumerating every 400 across the four routes
at the reviewed head 167d46c found four bare-string sites, not just DELETE's:

POST   /agents/profiles              InvalidProfileNameError
PUT    /agents/profiles/{name}       InvalidProfileNameError
DELETE /agents/profiles/{name}       InvalidProfileNameError
GET    /agents/profiles/{name}/source   ValueError

Only the validation path produced the dict. So the docstring was false in four
places, and the PR description carried the same claim. DELETE was the reachable
one for the reason you gave, that it has no body to validate first.

I took the other branch of your suggestion and made the claim true rather than
narrowing it. The shape moved to a module-level _profile_write_rejection and all
four use it, since an unsafe name is a rejected input in the same class as a
schema violation, and returning a dict for one and a string for the other is
exactly the type-switching the unification was meant to remove. PR C is the
client that would have paid for that. TestWriteRejectionShape is now
parameterized across POST, PUT and DELETE, including the service-raised
name error. 404 and 409 stay bare strings and that is now documented rather than
implied.

Worth naming the pattern rather than only the instance: this is the third
assertion in this PR that promised more than the code delivered, after the
replace_profile docstring and the concurrency test named for a deleter it never
exercised. Same failure mode each time, prose stating a contract with nothing
executable holding it to account. The parameterization is the actual fix.

Merge, since you flagged it needs separate validation

Merged origin/main at c64c9fa, which brings #526, #545 and #539. One conflict,
the typing import in api/main.py: this branch added Sequence and #526 added
AsyncIterator. Resolved to the union of both, then reformatted by black.
docs/api.md and test/api/test_scope_coverage.py auto-merged.

Verified on the merged tree:

• the five changed test modules plus test/cli/test_profile_cmd.py: 216 passed
• test/api/test_scope_coverage.py: 33 passed
• full suite: 6,691 passed
• black and isort clean across 548 files
• mypy on the changed validator: the two errors it reports are byte-identical at
  the prior head, so pre-existing

One aside in case you hit it too: test_scope_coverage.py had ten failures
locally against 0903561, all in the new workflow-run and terminal-output tests.
They are fixed upstream by c64c9fa and no longer reproduce on pristine main.

71 net new tests, rebaselined on c64c9fa since #526 added its own tests to
test_scope_coverage.py. The description has a Review round 2 section with the
detail.

Please review again - @haofeif and @fanhongy

P2a: URL-based MCP entries

@haofeif , You were right, and the codebase documented the contradiction itself. resolve_mcp_server_config's docstring says commandless {"type": "http", "url": ...} entries pass through untouched, while the schema had "required": ["command"] unconditionally, and this PR is what turned that into a blocking write gate.

Your exact example now validates and survives the round trip:

mcpServers: {docs: {type: http, url: https://mcp.example.invalid/mcp}}

  validator errors: 0
  parse_agent_profile_text: OK
  resolve_mcp_server_config -> {'type': 'http', 'url': 'https://mcp.example.invalid/mcp'}

mcpServers entries now require command or url via anyOf, with url declared as a property. Declaring it matters beyond documentation: the inner object doesn't set additionalProperties: false, so a url entry would have passed even undeclared, but then GET /agents/profiles/schema wouldn't describe the field and a wrong type wouldn't be a finding.

http url                -> accept
sse url                 -> accept
command                 -> accept
command + url together  -> accept
neither                 -> REJECT at mcpServers.docs
url: 7                  -> REJECT at mcpServers.docs.url

anyOf rather than oneOf deliberately, so an entry carrying both keys is still accepted. Nothing asked for exclusivity.

Two limits worth naming rather than leaving you to find:

url is the only spelling the branch accepts, and it appears exactly once in the repo as an MCP field name, in that docstring. An entry naming its endpoint as httpUrl or similar satisfies neither branch and is rejected. Still strictly better than before, which rejected every remote form, but it's a bet on one source. If the providers accept other spellings, say so and I'll widen it.

The rejection message for an entry with neither key is jsonschema's generic is not valid under any of the given schemas. It names the exact entry and path but not the missing key. if/then would produce 'url' is a required property, which reads better and is misleading, since command would also satisfy the rule. I kept the honest-but-vague message rather than adding message-rewriting to the validator. If you'd rather have the better text, expanding error.context for combinator failures is a small general change and I'll do it here.

P2b: bounded traversal, and the vector behind it

Your mechanism was exactly right and I reproduced it independently before changing anything: 640 bytes at 20 levels, ~1s, doubling per level, against ~0s for the jsonschema step beside it. Close enough to your 529 bytes / 1.5s to be the same thing. The depth cap bounded the wrong dimension. Depth was never the problem, revisiting shared objects was.

Of your three suggested remedies I took the first, tracking visited container identities, and added a global budget. I deliberately did not reject repeated or cyclic aliases. Anchor reuse is ordinary YAML and rejecting it would break documents that are fine:

toolsSettings:
  common: &c {timeout: 30}
  fs: *c
  web: *c          -> clean

toolsSettings: &s {self: *s}   (self-referential)  -> clean

Both are pinned by tests so a later "fix" can't satisfy the bound by refusing anchors.

Then I strawmanned that fix and found the larger half. jsonschema builds every error message eagerly, interpolating repr of the offending instance. So a document whose expansion is exponential in its byte count produces an error message that is too. Plant one type error whose instance is the amplified node:

2x per level, so ~26 levels reaches gigabytes, and that string gets serialised into the response. Allocation is the ceiling rather than CPU, and nothing done inside my traversal avoids it. On the 22-level document the identity-memoized walk costs 0.00003s while iter_errors is what explodes.

This one is pre-existing, not introduced by this PR. Verified against current origin/main at e1f6440 with the checkout's own source on PYTHONPATH so the tree under test is the one being imported:

imported from: <worktree>/src/cli_agent_orchestrator/services/profile_validator.py
has round-4 gate: False
levels=20 bytes=651 time=0.351s longest_msg=25,165,836
levels=22 bytes=713 time=1.422s longest_msg=100,663,308

The route was already scope-exempt and already async there, so this shipped with #575.

I fixed it here anyway. It's the same route and the same class of bug as the finding it sits behind, and telling you "the traversal is bounded" while a larger vector remained on that endpoint would have been half an answer. If you'd rather it went to its own issue against main so this PR stays scoped to the write endpoints, I'll split it out, no argument.

The final shape is one ceiling on expanded size, ahead of both the walk and the schema step, memoized on id() so the count stays linear in distinct objects and capped so an enormous document costs no more to reject than a borderline one. validate_frontmatter returns as soon as it reports, because continuing would pay exactly the cost the ceiling exists to avoid. After:

levels= 20  ->  1 finding, 171-char message
levels= 22  ->  1 finding, 171-char message
levels=200  ->  1 finding, 171-char message

Milliseconds, and flat as levels grow.

Two corrections to my own first attempt at this

Both were in the commit that fixed your finding, so they'd have landed under your name if you hadn't looked twice.

The budget decremented per edge traversed while being named _MAX_WALK_VALUES and reporting holds more than 20000 values. An 84 KB document holding four values, one of them aliased 21,000 times, was rejected for holding twenty thousand of them. Inside the 256 KB content cap, so reachable. Expressing the ceiling as expanded size fixed this at the root rather than by rewording: that document does expand to ~21,004 values, which is what the message now says.

The comment called both ceilings "~1000x" the largest bundled profile. Against its 23 expanded values and depth of 3 they are ~870x and ~21x. Wrong by roughly 50x for depth. Stated separately now.

I also documented something I'd left implicit: the two halves of validate_frontmatter report shared values differently. A shared value that's schema-invalid yields one finding per referencing path, since jsonschema doesn't memoize, while a shared non-string key yields exactly one. Both are correct, but a client rendering findings against a document shouldn't assume one convention.

Tests for all of this are deterministic rather than timed. The bomb is now rejected rather than traversed quickly, so the assertions are on the error message and on response size, under 2 KB for a body that previously returned 25 MB. If the memoization regresses, a test fails instead of hanging to the job timeout, which is what the earlier timing-only assertions would have done.

P2c

Noted, and thanks for revisiting it. No action taken. The Known gaps entry on placeholder-aware validation stays as it was, and I agree it wants its own definition rather than riding along here.

One interaction with #606

#606 gated GET /agents/profiles and GET /agents/profiles/{name}, which are the pre-existing ungated siblings this PR deliberately left alone when it gated /source, on the #505 precedent that tightening a shipped route risks breaking an existing unauthenticated reader. That asymmetry is gone now, so the rationale recorded on the route and in docs/api.md said something that stopped being true at the merge. Both rewritten.

I also added GET /agents/profiles/{name}/source to #606's _GATED_ROUTES and its sample requests, so it's covered by that file's auth-enabled enforcement tests instead of only by this PR's structural one. It belongs there on #606's own definition, arguably more than the parsed route does, since it returns stored bytes verbatim from the local, provider, extra and built-in stores including documents that fail to parse. That list is hand-maintained by design, so a route added later doesn't join it automatically. Flagging it rather than editing another PR's test file quietly.

Verification

origin/main is merged in as of e1f6440, so #608, #622 and #596 are included. #596 edits the same profile schema this PR does, in a different region, and merged clean; I checked that both sides' additions are present, that the #575 schema/model parity test still passes, and that #596's own profile validates clean through the ceiling added here.

The PR description is rewritten with a Review round 4 section carrying the measurements above.

Severity correction after impact/likelihood triage: the scalar-alias amplification is P1 because one unauthenticated, under-limit request can force multi-gigabyte allocation and take down the server. The cyclic-YAML finding is P3 / non-blocking because it requires deliberately self-referential input and affects only that profile. The Changes Requested verdict remains because the P1 is independently blocking.

Both fixed, and both were mine. I reproduced each on 8bda8ec before changing anything, and one of them contradicts something I told you in my last reply, so I want to be direct about that first.

Correction to my round-4 reply

@haofeif , I said I had deliberately declined your "reject repeated/cyclic aliases" suggestion, and I cited the self-referential document validating clean as evidence that this was the right call. That reasoning holds for acyclic reuse and was wrong for cycles, and you were right to have grouped them in the original suggestion. They are handled separately now.

Cycles were treated as finite, and persisted

The provisional memo entry gave a back-edge a size of 1, so the document counted as small:

1. validator: CLEAN (valid)
2. cycle survives parsing: child-is-grandchild=True
3. KiroAgentConfig(...).model_dump_json() RAISES
   PydanticSerializationError: Circular reference detected (id repeated)

So POST returned 201 and wrote a profile that install_service.py:379 cannot serialize. That is the same failure mode as the non-string mapping key rule from two rounds earlier: parseable enough to accept, unusable once written. Worse, round 4's own test asserted the cyclic document was valid, so the mistake was pinned in place rather than merely present.

Fixed in the shape you described. In-progress identities are tracked separately from completed ones, so revisiting a container still being measured is a back-edge and an error, while revisiting a finished one is ordinary sharing and stays memoized. Acyclic reuse still validates clean, which is the property I was trying to protect and which a test now holds me to.

While probing it I found the back-edge does not have to be a mapping, so hooks: &a [*a] and hooks: &a [{inner: *a}] are covered too. Only the mapping form was reported.

The guard counted the wrong unit

Your mechanism is exactly right: every scalar returned 1 regardless of length, while the cost is repr(instance), which re-renders the scalar's bytes at every alias. Both your cases, reproduced:

My first figure differs from your 10,260,109 by 85 bytes, which I put down to formatting in how I built the document rather than a different effect.

The ceiling is now in rendered bytes, the unit the downstream step actually pays in. A scalar contributes its own rendered length, a mapping its keys and separators, and a sequence its separators. The guard computes each object's repr cost once and caches that length, then charges the cached length at every reference. That bounds the guard's own allocation without undercounting what downstream rendering would print. Set at 1 MB, which is about 3.8x the largest request that can arrive given the 256 KB cap on content, since a document with no aliasing renders to roughly its own size. For scale, the largest bundled profile renders to 485 bytes. Both of your cases now return a 227-byte rejection, and the round-3 anchor bombs still return the same at 200 levels.

The part I want to name rather than let you find. This is the second time this function has bounded the wrong dimension. Round 3 capped depth when the cost was repeat visits; round 4 capped occurrences when the cost was bytes. Two wrong dimensions in a row is a reason not to trust that the third is right, so I also took your alternative and bounded the output: schema findings are length-capped before they reach a response. To be clear about what that does and does not do, it is a backstop and not the fix. jsonschema builds the message inside iter_errors, so the allocation has already happened by the time anything of mine sees the string. What the cap does buy is the case the ceiling does not cover: a document within the rendering ceiling can still trip several schema errors. Each finding, including its truncation marker, is now capped at exactly 2,000 characters. Post-fix regression probes covered many-error, long-path and shared-invalid-subtree shapes; finding count still requires corresponding invalid structure in the request, and HTTP aggregates stayed below 300 KB under the 256 KB content cap. No unbounded response amplification remained.

Those probes found one P3 in my first backstop: it retained 2,000 characters and then appended a 35--38 character marker. The marker is now budgeted inside the 2,000-character total, and an exact-boundary test pins that contract. Across the reviewer cases and additional graph shapes, peak memory stayed below 15 MB; no P1/P2 bypass remained.

Also not reported and found while probing: YAML merge keys are a second alias mechanism. <<: *anchor copies the target's entries into each merging mapping, so 3,000 merge-key copies of a 60 KB target multiply content by a different route than an aliased scalar. Rejected now, with ordinary merge keys asserted to still pass.

What is pinned so this does not regress

The self-referential test inverts from accepted to rejected, and a companion asserts the runtime genuinely cannot serialize it, so the rule reads as a reason rather than a restriction. Both of your scalar cases are asserted on response size. Cycles are parameterized over the mapping, sequence, and sequence-to-mapping shapes. A large unaliased document and ordinary merge keys are both asserted to still pass, so the bound cannot be satisfied by rejecting size or aliasing outright.

Focused profile-validator/API suite: 139 passed. Full suite: 7,023 passed, with the same 62 known local-only failures outside this surface. 111 net new tests, measured per file with --collect-only against current origin/main at 3c2f559, which is merged in. Black and isort are clean across 556 files; targeted mypy has only the two pre-existing validator errors (missing jsonschema stubs and an Any return).

Re-reviewed current head aebc405 against all prior iterations. Earlier blockers remain fixed, but one P1 remains.

[P1] Bound aggregate validation findings before materialization — src/cli_agent_orchestrator/services/profile_validator.py:374-397 still eagerly sorts every JSON Schema error and appends one warning per unknown allowedTools entry without a shared count or byte budget. On this head, 10,000 entries produced 10,000 findings and a 1,750,029-byte response; the scope-exempt synchronous endpoint therefore remains vulnerable to request-sized amplification and event-loop or memory exhaustion. Stop each producer under a shared aggregate budget and reserve an omission marker.

Fixed in the latest update. Aggregate validation findings now use a shared budget of 100 findings and 100 KB across all producers. Schema errors are bounded before sorting/materialization, unknown allowedTools warnings stop at the shared limit, and capacity is reserved for an omission marker.

I also added large-input endpoint regressions, a real-validator laziness test confirming only the bounded prefix is consumed, and cross-process determinism coverage. The focused service/API suite passes with 147 tests.

@haofeif Thanks for catching this. Addressed in 4bf7c2e.

Validation findings now share a request-wide limit of 100, including findings from the custom additionalProperties validator. Traversal stops as soon as the budget is exhausted, preventing request-sized CPU and memory amplification while preserving deterministic document order.

I added regression coverage for:

Aggregate request-wide finding limits
Lazy traversal through the real validator using CountingDict
Deterministic output with reverse insertion order
Bounded API error responses
I also fixed an inherited test-isolation issue in 4a6e8fd: the extraction-settlement test now uses a unique workflow run ID so persisted journal state cannot change its expected path.

Validation is green:

225 focused tests passed
Full suite: 7,966 passed, 29 skipped, 105 deselected, 1 xfailed
Black, isort, and git diff --check passed
Could you please take another look when you have time?